-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Change annotation syntax to use @
#2729
Conversation
This is not great code — likely needs some error handling, and maybe some changes to how it's implemented in Chumsky. But it works!
Sorry, there's no linked issue so asking here: was there a discussion around No preference from my side at this stage but just curious what the considerations would be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Annotations can now be arbitrary expressions - literals, function calls, tuples, lists.
And SQL backend expects only tuples.
We mentioned here: #2694 |
Some thoughts that came to me on this are that
So |
@@ -7,5 +7,5 @@ SELECT | |||
FROM | |||
tracks | |||
WHERE | |||
name ~ '\(I Can''t Help\) Falling' | |||
(name) ~ ('\(I Can''t Help\) Falling') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I saw these too — I'm not sure why they happen...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry!
(I blame the comment syntax :D )
I cleaned up the code a bit, and merging. I don't know why we're getting these extra parentheses at #2729 (comment) — I don't even see where it could be affecting this! Not super important so I won't spend time on it, lmk any dissent |
In the future could there be other types of annotations? Maybe The syntax is rather unorthodox, hence I think using square brackets instead of curly brackets would be preferred as it would be more familiar. |
Syntax for annotations is just:
... where In any case, this was added mostly as an internal detail. It is not documented and especially |
This is not great code — likely needs some error handling, and maybe some changes to how it's implemented in Chumsky. But it works!